feat(record-tour): save and load tour setup as a JSON file - #898
Conversation
The Record Map Tour panel could only export the recorded video; the underlying keyframes, durations, and frame rate were lost when the panel closed, so a tour could not be paused, refined, or reused. Add a Save setup / Load setup pair near the top of the panel. Save writes the keyframes and FPS to a JSON file; Load reads one back, repopulating the keyframe list and frame rate (fresh ids are minted so reloaded rows never collide). The serializer and parser share the FPS/segment bounds with the controls, so a hand-edited or stale file is clamped to the supported range, and a malformed file surfaces a translated error instead of crashing. Closes #897
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
More reviews will be available in 18 minutes and 7 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesTour configuration save/load
Sequence Diagram(s)sequenceDiagram
participant RecordTourDialog
participant tour_recorder.ts
participant saveTextFileWithFallback
participant openLocalDataFileWithFallback
RecordTourDialog->>tour_recorder.ts: serializeTourConfig(keyframes, fps)
RecordTourDialog->>saveTextFileWithFallback: write config JSON
RecordTourDialog->>openLocalDataFileWithFallback: choose config file
openLocalDataFileWithFallback-->>RecordTourDialog: file text
RecordTourDialog->>tour_recorder.ts: parseTourConfig(text)
tour_recorder.ts-->>RecordTourDialog: parsed keyframes and fps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewReviewed Four findings below, from most to least significant. Bugs / Logic
Quality
CLAUDE.md (i18n)
No security issues found. The JSON parsing is properly guarded with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/lib/tour-recorder.ts`:
- Around line 185-208: The parseTourConfig function currently validates type and
keyframes but ignores the serialized version field, so add a version check
before parsing the rest of the payload. In parseTourConfig, verify obj.version
against TOUR_CONFIG_VERSION and throw the same kind of user-facing error used
for other invalid tour config cases when it does not match, so future
incompatible formats fail fast instead of being parsed incorrectly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32c8cae5-f881-45a7-b535-f695a9bac44c
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/tour-recorder.tstests/tour-recorder.test.ts
- parseTourConfig now rejects a config written by a newer, incompatible
format version (was stamped on save but never read back); a missing or
older version is still accepted.
- Clamp zoom/pitch/bearing into MapLibre's supported ranges on load to match
the documented validation contract (only durationMs/fps were clamped).
- Rename the config file default to "map-tour-setup" so it is genuinely
distinct from the video name, and fix the misleading comment.
- Grammar: "Saved setup as {{name}}".
- Add tests for camera clamping and newer-version rejection.
Code reviewOverall this is a clean, well-tested addition. The serialization/parsing logic is solid, the constants are properly centralised, and the i18n integration is correct. The four findings below are the things I would address before merging. Bugs
Security / Performance
Quality / UX
CLAUDE.mdNo violations. New user-facing strings are correctly behind |
- Wrap bearing onto (-180, 180] instead of clamping, so a hand-edited 270 maps to -90 (west) rather than 180 (south). - Reject a keyframe whose latitude is outside ±90 (a real out-of-range coordinate), matching the validation the comment claims. - Cap parsed keyframes at 500 so a crafted/huge file can't make the parser allocate a giant array and loop createId() over it. - handleLoadConfig: only clear the result banner once a file is actually chosen, so cancelling the picker no longer wipes a prior "Saved setup…". - Confirm before loading when the panel already has keyframes, so a misclick on "Load setup" can't silently discard in-progress work (new confirmLoad string). - Tests for bearing wrap, latitude rejection, and the keyframe cap.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx (1)
378-396: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve banners when config save is canceled.
Line 380 clears the current result before the save dialog resolves. If the user cancels,
nameisnulland the prior “saved/loaded” message is lost even though nothing changed.Suggested fix
const handleSaveConfig = async () => { if (keyframes.length === 0) return; - clearResultMessages(); try { const content = serializeTourConfig(keyframes, fps); const fileType = t("recordTour.configFileType"); const name = await saveTextFileWithFallback(content, { @@ ], mimeType: "application/json", }); - if (name) setConfigMessage(t("recordTour.configSaved", { name })); + if (!name) return; + clearResultMessages(); + setConfigMessage(t("recordTour.configSaved", { name })); } catch (err) { console.warn("Tour configuration save failed", err); + clearResultMessages(); setError(t("recordTour.configSaveError")); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx` around lines 378 - 396, In handleSaveConfig, avoid clearing the current result message before the save dialog outcome is known, because canceling the dialog currently wipes the existing banner even though no config change occurred. Move clearResultMessages() so it only runs after saveTextFileWithFallback returns a real filename, or otherwise restore the previous message when name is null; keep the behavior scoped to RecordTourDialog’s handleSaveConfig, setConfigMessage, and saveTextFileWithFallback flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/lib/tour-recorder.ts`:
- Around line 51-55: The MAX_KEYFRAMES guard in tour-recorder.ts is applied
after JSON.parse, so large config text can still be fully allocated before
rejection. Add an upfront text-length guard in the config import/parsing flow
before calling JSON.parse, and keep the existing MAX_KEYFRAMES validation
afterward; use the relevant parsing/import path around the keyframes handling so
the limit protects the parser as well as the later loop that mints ids.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsx`:
- Around line 378-396: In handleSaveConfig, avoid clearing the current result
message before the save dialog outcome is known, because canceling the dialog
currently wipes the existing banner even though no config change occurred. Move
clearResultMessages() so it only runs after saveTextFileWithFallback returns a
real filename, or otherwise restore the previous message when name is null; keep
the behavior scoped to RecordTourDialog’s handleSaveConfig, setConfigMessage,
and saveTextFileWithFallback flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 57d5220c-65d9-4abe-be47-f0f6c67acb6e
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/layout/RecordTourDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/tour-recorder.tstests/tour-recorder.test.ts
- Guard the raw config text length (1 MB) before JSON.parse, so a pathological file is rejected without being fully allocated first (completes the MAX_KEYFRAMES DoS hardening, which only ran post-parse).
Code reviewBugs
Quality
Security, Performance, CLAUDE.mdNothing significant. The parser is well-hardened: the |
- handleSaveConfig now clears the result banner only after the file is actually written, matching handleLoadConfig, so cancelling the save dialog no longer wipes a prior "Saved setup…" message. - handleLoadConfig short-circuits only on a cancelled picker (null result); an empty file now flows through to parseTourConfig so it surfaces a real error instead of silently doing nothing.
Code reviewOverall the implementation is solid: the serialize/parse pair is well-designed with sensible limits (1 MB, 500 keyframes), the validation is thorough, bearing normalization is mathematically correct, constants are properly centralised, and the test suite covers round-trips, clamping, and rejection paths well. Three findings below, all in the newly introduced code. Bugs
Quality / Maintainability
SecurityNothing to flag. The 1 MB text-length guard before PerformanceNo concerns. The size and keyframe-count limits are appropriately conservative. CLAUDE.md
|
Code reviewOverall this is a clean, well-scoped addition. The parser is defensive (text-length cap, keyframe count cap, clamping, bearing normalisation, lat range check), the constants are correctly centralised, and the test suite covers the main cases. Three findings below. Bugs
Quality
SecurityNothing to flag. The text-length cap (1 MB), keyframe count cap (500), input clamping, and absence of PerformanceNothing to flag. Serialising / parsing even the maximum 500-keyframe config is well within synchronous budget. CLAUDE.mdNew i18n strings are added under |
- Clamp each keyframe's durationMs on serialize too, mirroring parseKeyframe, so save/load is symmetric and a programmatic caller can't persist an out-of-range duration. - Tighten the version gate to also reject a present-but-non-numeric version (e.g. "2"); a missing version is still accepted as legacy v1. Add tests for the string-version rejection and the missing-version acceptance.
- Fix openLocalDataFileWithFallback hanging forever when the browser file picker is dismissed without a selection: the input only had an onchange handler (which never fires on cancel), so handleLoadConfig's await never settled. Add a "cancel" listener that resolves null, matching the existing pickImageFilesWithFallback pattern. - Assert the missing-fps fallback to DEFAULT_FPS in the parse test.
| ): string { | ||
| const config: TourConfig = { | ||
| type: TOUR_CONFIG_TYPE, | ||
| version: TOUR_CONFIG_VERSION, |
There was a problem hiding this comment.
Quality (low confidence): camera fields are not normalized on save, creating a subtle round-trip asymmetry
The comment just above this block says duration clamping is added "so save/load is symmetric", but ...rest passes center, zoom, pitch, and bearing through with their original full-precision values. parseKeyframe then rounds them on load (roundTo(center[0], 6), roundTo(zoom, 3), roundTo(pitch, 1), roundTo(bearing, 1)).
In practice the live map values are already limited in precision and the rounding is sub-perceptible, so this won't cause visible drift. But if the consistency guarantee ever matters (e.g. a test that asserts exact round-trip equality for high-precision synthetic values), the saved file will pass the test and the loaded values will differ by rounding.
If you want full symmetry, apply the same normalisation on write:
keyframes: keyframes.map(({ id: _id, durationMs, center, zoom, pitch, bearing }) => ({
center: [roundTo(center[0], 6), roundTo(center[1], 6)] as [number, number],
zoom: roundTo(clampNumber(zoom, 0, MAX_ZOOM), 3),
pitch: roundTo(clampNumber(pitch, 0, MAX_PITCH), 1),
bearing: roundTo(normalizeBearing(bearing), 1),
durationMs: clampNumber(Math.round(durationMs), MIN_SEGMENT_SECONDS * 1000, MAX_SEGMENT_SECONDS * 1000),
})),
| /** The on-disk shape of a saved tour configuration. */ | ||
| export interface TourConfig { | ||
| type: string; | ||
| version: number; |
There was a problem hiding this comment.
Quality — TourConfig.version declared required but parsed as optional
The interface declares version: number (required), but parseTourConfig explicitly accepts files with no version field and the test "accepts a file with no version field (legacy / hand-written)" exercises that path. The on-disk type should reflect the actual accepted shape:
| version: number; | |
| version?: number; |
Without the ?, any code that constructs a TourConfig object without version would get a TypeScript error even though the parser deliberately allows it.
| }; | ||
|
|
||
| // Load a previously saved tour setup, replacing the current keyframe list and | ||
| // frame rate. Fresh ids are minted so reloaded rows never collide. A bad file |
There was a problem hiding this comment.
Quality (low confidence): window.confirm shows a native OS dialog that may be suppressed in some contexts
window.confirm is synchronous and displays the browser/OS native dialog, which can be blocked in cross-origin iframes or certain embedded contexts (e.g. some Jupyter environments). It also does not respect the app's design system or dark theme.
The existing codebase's tauri-io.ts already provides ask() from @tauri-apps/plugin-dialog (for Tauri) and presumably a fallback, which would give a better-integrated confirmation. Alternatively, an inline "confirm" banner (e.g. a yellow warning that appears above the button row when keyframes exist, with a "Load anyway" secondary button) would avoid the modal entirely.
This is a deliberate tradeoff and the current implementation is safe — just flagging it for the design review pass.
| @@ -92,6 +104,9 @@ export function RecordTourDialog({ | |||
| const [error, setError] = useState<string | null>(null); | |||
| const [savedName, setSavedName] = useState<string | null>(null); | |||
| const [saveCancelled, setSaveCancelled] = useState(false); | |||
There was a problem hiding this comment.
Nit: comment overstates the separation guarantee
The comment says the config banner is "kept separate from the video save banner so the two messages never clobber each other". In fact they can coexist in the DOM simultaneously (good), but both are erased by clearResultMessages() whenever any new operation starts — so a "Saved setup as…" banner does disappear if the user immediately starts a recording.
The comment would be more accurate as: "kept separate so both can display simultaneously; cleared together when any new operation begins."
This is cosmetic only.
| clearResultMessages(); | ||
| setConfigMessage(t("recordTour.configSaved", { name })); |
There was a problem hiding this comment.
Bug — clearResultMessages() wipes the "Video saved as…" banner on config save
clearResultMessages() zeros out savedName (the video-saved notification) along with the config message and any error. Saving the setup doesn't change the tour, so the docstring on clearResultMessages says it should only be called by edits that actually change the tour — this call is outside that contract.
A user who records a tour, saves the video ("Saved as map-tour.webm"), and then immediately clicks "Save setup" will see the video-saved banner silently disappear even though nothing in the tour changed.
The fix is to clear only the states that are directly relevant here (the previous config banner and any lingering load/save error) while leaving savedName intact:
| clearResultMessages(); | |
| setConfigMessage(t("recordTour.configSaved", { name })); | |
| setError(null); | |
| setConfigMessage(t("recordTour.configSaved", { name })); |
(The new setConfigMessage(...) call already replaces the old config message, so there's no need to explicitly null it first.)
Code reviewOverall this is a well-structured feature: the serialization/parsing logic is solid, the bounds are shared between the UI and the parser, the file size and keyframe-count guards prevent DoS from crafted inputs, and the test suite covers the important edge cases (round-trip, clamping, version gating, malformed input). Two findings worth addressing are below. Bugs
Quality
What I checked and found clean
|
- Tighten the version gate: a present version must be an integer in [1, TOUR_CONFIG_VERSION], so an unrecognized 0 or negative value is now rejected too (only a missing version still defaults to v1). Add a test for the version-below-1 case.
| // Drop the id; clamp the duration on write too (mirroring parseKeyframe) so | ||
| // save/load is symmetric and a programmatic caller can't persist an | ||
| // out-of-range value. | ||
| keyframes: keyframes.map(({ id: _id, durationMs, ...rest }) => ({ | ||
| ...rest, | ||
| durationMs: clampNumber( | ||
| Math.round(durationMs), | ||
| MIN_SEGMENT_SECONDS * 1000, | ||
| MAX_SEGMENT_SECONDS * 1000, | ||
| ), | ||
| })), |
There was a problem hiding this comment.
The comment claims the write-time clamping prevents a "programmatic caller can't persist an out-of-range value," but only durationMs is clamped here — zoom, pitch, and bearing are spread verbatim via ...rest. A programmatic caller supplying zoom: 200 would write that to disk; on re-read parseKeyframe would clamp it to 24, so the round-trip is silent-lossy for camera values. The stated symmetry goal also isn't fully met: parse(serialize(kf)) ≠ kf if the input already carries rounding artefacts (e.g. zoom: 12.5000001), because the parser runs roundTo but the serializer doesn't.
In practice the UI only creates keyframes from MapLibre's live camera, so values are always in range — low-severity day-to-day — but applying the same normalization on write would make the guarantee watertight:
| // Drop the id; clamp the duration on write too (mirroring parseKeyframe) so | |
| // save/load is symmetric and a programmatic caller can't persist an | |
| // out-of-range value. | |
| keyframes: keyframes.map(({ id: _id, durationMs, ...rest }) => ({ | |
| ...rest, | |
| durationMs: clampNumber( | |
| Math.round(durationMs), | |
| MIN_SEGMENT_SECONDS * 1000, | |
| MAX_SEGMENT_SECONDS * 1000, | |
| ), | |
| })), | |
| // Drop the id; clamp and normalize on write too (mirroring parseKeyframe) so | |
| // save/load is symmetric and a programmatic caller can't persist an | |
| // out-of-range value. | |
| keyframes: keyframes.map(({ id: _id, durationMs, ...rest }) => ({ | |
| center: [roundTo(rest.center[0], 6), roundTo(rest.center[1], 6)] as [number, number], | |
| zoom: roundTo(clampNumber(rest.zoom, 0, MAX_ZOOM), 3), | |
| pitch: roundTo(clampNumber(rest.pitch, 0, MAX_PITCH), 1), | |
| bearing: roundTo(normalizeBearing(rest.bearing), 1), | |
| durationMs: clampNumber( | |
| Math.round(durationMs), | |
| MIN_SEGMENT_SECONDS * 1000, | |
| MAX_SEGMENT_SECONDS * 1000, | |
| ), | |
| })), |
| const handleLoadConfig = async () => { | ||
| // Loading replaces the whole tour, so confirm first when there is existing | ||
| // work a misclick would otherwise wipe. | ||
| if (keyframes.length > 0 && !window.confirm(t("recordTour.confirmLoad"))) { |
There was a problem hiding this comment.
window.confirm() is suppressed in cross-origin iframes in Chrome 92+ and modern Firefox — it returns false without ever showing a dialog. The app is embedded in Jupyter notebooks (per CLAUDE.md), where the widget iframe is cross-origin, so any user who has added even one keyframe inside a Jupyter cell would find the Load button silently broken: the confirm call returns false, the function returns early, and no file picker opens with no explanation shown.
The existing window.confirm calls in DesktopShell.tsx and StoryMapPanel.tsx follow the same pattern, so this is consistent with the codebase — but RecordTourDialog is exposed in the Jupyter embed while those files arguably aren't. Consider replacing with a small non-blocking in-app confirm using an existing shadcn AlertDialog, or at minimum falling through to the file picker when window.confirm is not available:
| if (keyframes.length > 0 && !window.confirm(t("recordTour.confirmLoad"))) { | |
| if (keyframes.length > 0) { | |
| // window.confirm is suppressed in cross-origin iframes (e.g. Jupyter). | |
| // When it returns false and was actually suppressed (not explicitly | |
| // cancelled), we'd silently block the load — fall through instead. | |
| const dialogAvailable = | |
| typeof window.confirm === "function" && | |
| !window.top?.location.origin !== window.location.origin; // cross-origin | |
| if (dialogAvailable && !window.confirm(t("recordTour.confirmLoad"))) { | |
| return; | |
| } | |
| } |
(The iframe-detection heuristic is imperfect; a proper shadcn AlertDialog is the cleaner fix, but the fallback above would at least stop silently blocking the action.)
| }; | ||
| // Resolve (rather than hang) when the dialog is dismissed without a pick; | ||
| // `change` never fires on cancel, so without this the Promise never settles. | ||
| input.addEventListener("cancel", () => resolve(null)); |
There was a problem hiding this comment.
Good fix for the hanging-promise bug. Worth noting: the cancel event on <input type="file"> shipped in Chrome 113, Firefox 113, and Safari 16.4 (all April–September 2023). On older browsers the cancel event never fires and the promise still won't settle on dismiss. If pre-2023 browser support is in scope, a complementary focus / visibilitychange heuristic would be needed. Not a blocker — this is strictly an improvement over the status quo.
| throw new Error("File is not a GeoLibre tour configuration."); | ||
| } | ||
| // Reject a file written by a newer, incompatible format so its data isn't | ||
| // silently misread. A missing version is accepted (hand-written/legacy files |
There was a problem hiding this comment.
Nits (group):
-
num(kf.bearing)usesfallback = 0(north), which is a reasonable choice for a missing bearing, but is worth a brief comment since0for bearing is not as obvious a default as it is for zoom/pitch. -
The
as numbercast oncenter[0]andcenter[1]at line 234 is safe becauseNumber.isFinitealready guards the path — fine as-is, just noting the implicit assumption for future readers. -
The
TourConfiginterface (type: string) is slightly looser than what the parser enforces (type === TOUR_CONFIG_TYPE). Usingtype: typeof TOUR_CONFIG_TYPEor a literal type would let TypeScript catch a mis-spelled marker at call sites, though in practiceserializeTourConfigis the only author.
Code reviewBugs
PerformanceNothing to raise. SecurityNothing to raise. The size guard ( Quality
CLAUDE.mdNo violations. Constants correctly moved to Overall this is well-structured work with solid validation and test coverage. The two medium-confidence bug findings are the ones most worth addressing before merge. |
Code review\n\nReviewed tour-recorder.ts |
1 participant
Summary
The Record Map Tour panel could only export the finished video. The underlying tour data (keyframes, per-segment transition durations, frame rate) lived only in component state and was lost when the panel closed, so a tour could not be paused, refined, or reused in a later session. This implements the save/load requested in #897.
What changed
.jsonfile (via the existingsaveTextFileWithFallback, so it works in both the desktop save dialog and the browser download/File System Access fallback).geolibre-tourmarker + schema version in the file, with a parser that validates structure, requires at least one keyframe, and clamps the frame rate and every segment duration into the same range the controls enforce (so a hand-edited or stale file is never out of range). A malformed file shows a translated error rather than crashing.tour-recorder.tsas exported constants so the UI and the parser share one source of truth.recordTour(en.json).serializeTourConfig/parseTourConfig(round-trip, clamping, and rejection of malformed input).Verification
npm run buildandnode --test tests/tour-recorder.test.tspass;pre-commitclean on the changed files.Closes #897
Summary by CodeRabbit